.../input/entityanalytics/provider/okta: Rate limiting fix, improvements #41977

chrisberkhout · 2024-12-10T19:32:08Z

Proposed commit message

.../input/entityanalytics/provider/okta: Rate limiting fix, improvements

- Fix a bug in the stopping of requests when `x-rate-limit-remaining: 0`.
- Add a deadline so long waits return immediately as errors.
- Add an option to set a fixed request rate.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Relates x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting hangs #40106

…d reactivating later.

elasticmachine · 2024-12-10T19:32:11Z

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

mergify · 2024-12-10T19:32:43Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @chrisberkhout? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-12-10T19:32:43Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

efd6 · 2024-12-10T21:13:26Z

x-pack/filebeat/input/entityanalytics/provider/okta/conf.go

@@ -27,6 +27,7 @@ func defaultConfig() conf {
 		SyncInterval:   24 * time.Hour,
 		UpdateInterval: 15 * time.Minute,
 		LimitWindow:    time.Minute,
+		LimitFixed:     nil,


This isn't needed. Is it here for explicitness?

I guess I was thinking out loud. Doesn't need to be there. Removed.

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

efd6 · 2024-12-10T21:33:02Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

+	ready := make(chan struct{})
+	close(ready)


Suggest something like

// alwaysReady is an non-blocking chan. var alwaysReady = make(chan struct{}) func init() { close(alwaysReady) } func (r RateLimiter) endpoint(path string) endpointRateLimiter { if existing, ok := r.byEndpoint[path]; ok { return existing } limit := rate.Limit(1) if r.fixedLimit != nil { limit = rate.Limit(float64(*r.fixedLimit) / r.window.Seconds()) } limiter := rate.NewLimiter(limit, 1) // Allow a single fetch operation to obtain limits from the API newEndpointRateLimiter := endpointRateLimiter{ limiter: limiter, ready: alwaysReady, } r.byEndpoint[path] = newEndpointRateLimiter return newEndpointRateLimiter }

so that we have a single never-blocking chan.

Done with the name immediatelyReady.
It's true that channel will always be ready, but the endpointRateLimiter's ready field won't always have that value...
I don't know. Is this better, or the same? (or less good?)

efd6 · 2024-12-10T21:39:16Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

-	log.Debugw("rate limit", "limit", limiter.Limit(), "burst", limiter.Burst(), "url", url.String())
-	return limiter.Wait(ctx)
+	e := r.endpoint(endpoint)
+	<-e.ready


Suggested change

<-e.ready

<-e.ready

select {

case <-e.ready:

case <-ctx.Done():

return ctx.Err()

}

efd6 · 2024-12-10T21:47:54Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter.go

+	e := r.endpoint(endpoint)
+	<-e.ready
+	log.Debugw("rate limit", "limit", e.limiter.Limit(), "burst", e.limiter.Burst(), "url", url.String())
+	ctxWithDeadline, cancel := context.WithDeadline(ctx, time.Now().Add(waitDeadline))


Should the deadline be calculated before the e.ready recv?

I intended to apply it only when using the rate limiter, not when requests are shut down, but I'll change it to apply to both.

efd6 · 2024-12-10T21:56:13Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter_test.go

+		target := 30.0
+		buffer := 0.01
+
+		if tokens < target-buffer || tokens > target+buffer {


Suggested change

if tokens < target-buffer || tokens > target+buffer {

if tokens < target-buffer || target+buffer < tokens {

efd6 · 2024-12-10T21:56:40Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter_test.go

+		wait := time.Since(start)
+
+		if wait > 1010*time.Millisecond {
+			t.Errorf("doesn't allow requests to resume after reset. had to wait %d milliseconds", wait.Milliseconds())


Suggested change

t.Errorf("doesn't allow requests to resume after reset. had to wait %d milliseconds", wait.Milliseconds())

t.Errorf("doesn't allow requests to resume after reset. had to wait %s", wait)

efd6 · 2024-12-10T21:58:47Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter_test.go

 		const endpoint = "/foo"
-		limiter := r.limiter(endpoint)
+		url, _ := url.Parse(endpoint)


Suggested change

url, _ := url.Parse(endpoint)

url, err := url.Parse(endpoint)

if err != nil {

t.Fatalf("failed to parse endpoint: %v", err)

}

efd6 · 2024-12-10T21:59:07Z

x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/ratelimiter_test.go

+
+		const endpoint = "/foo"
+
+		url, _ := url.Parse(endpoint)


Check error.

…ests.

chrisberkhout · 2024-12-11T17:31:24Z

@efd6 Addressed comments. Also added documentation changes that I just forgot to commit before. Now also has CHANGELOG.next.asciidoc entries (1 bugfix, 1 addition/improvement).

mergify · 2024-12-11T17:32:02Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b ea-okta-rate-limiting-operability upstream/ea-okta-rate-limiting-operability
git merge upstream/main
git push upstream ea-okta-rate-limiting-operability

efd6

Thanks

…nts (#41977) - Fix a bug in the stopping of requests when `x-rate-limit-remaining: 0`. - Add a deadline so long waits return immediately as errors. - Add an option to set a fixed request rate. (cherry picked from commit 08b7d84)

…limiting fix, improvements (#42008) * .../input/entityanalytics/provider/okta: Rate limiting fix, improvements (#41977) - Fix a bug in the stopping of requests when `x-rate-limit-remaining: 0`. - Add a deadline so long waits return immediately as errors. - Add an option to set a fixed request rate. (cherry picked from commit 08b7d84) --------- Co-authored-by: Chris Berkhout <[email protected]>

…nts (elastic#41977) - Fix a bug in the stopping of requests when `x-rate-limit-remaining: 0`. - Add a deadline so long waits return immediately as errors. - Add an option to set a fixed request rate.

chrisberkhout added 3 commits December 9, 2024 11:05

Set a deadline for Wait.

cbbe461

RateLimiter handles blocking when there are no remaining requests, an…

b80729a

…d reactivating later.

Add a configuration option to override with a constant rate.

3ba9e5b

chrisberkhout added enhancement bugfix Team:Security-Service Integrations Security Service Integrations Team labels Dec 10, 2024

chrisberkhout self-assigned this Dec 10, 2024

chrisberkhout requested a review from a team as a code owner December 10, 2024 19:32

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Dec 10, 2024

chrisberkhout mentioned this pull request Dec 10, 2024

x-pack/filebeat/input/entityanalytics/provider/okta: Rate limiting hangs #40106

Open

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Dec 10, 2024

efd6 reviewed Dec 10, 2024

View reviewed changes

chrisberkhout added 9 commits December 11, 2024 09:25

Doc update.

799c175

Remove redundant nil from defaultConfig().

3376731

Godoc and naming.

80cb354

Make fewer channels.

be813e8

Apply deadline and handle its expiry also during blocking of all requ…

4d26f53

…ests.

Check errors from url.Parse().

c02c8e9

Nicer test error message formatting.

996fc71

Nicer threshold check in test.

18c8e96

CHANGELOG.next.asciidoc entries.

73cd8f6

chrisberkhout requested a review from efd6 December 11, 2024 17:31

Merge branch 'main' into ea-okta-rate-limiting-operability

c5ccfc7

efd6 approved these changes Dec 11, 2024

View reviewed changes

chrisberkhout merged commit 08b7d84 into elastic:main Dec 12, 2024
20 of 22 checks passed

mergify bot mentioned this pull request Dec 12, 2024

[8.x](backport #41977) .../input/entityanalytics/provider/okta: Rate limiting fix, improvements #42008

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.../input/entityanalytics/provider/okta: Rate limiting fix, improvements #41977

.../input/entityanalytics/provider/okta: Rate limiting fix, improvements #41977

chrisberkhout commented Dec 10, 2024 •

edited

Loading

elasticmachine commented Dec 10, 2024

mergify bot commented Dec 10, 2024

mergify bot commented Dec 10, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

efd6 Dec 10, 2024

chrisberkhout Dec 11, 2024

chrisberkhout commented Dec 11, 2024

mergify bot commented Dec 11, 2024

efd6 left a comment

-	<-e.ready
+	<-e.ready
+	select {
+	case <-e.ready:
+	case <-ctx.Done():
+		return ctx.Err()
+	}

	if tokens < target-buffer \|\| tokens > target+buffer {
	if tokens < target-buffer \|\| target+buffer < tokens {

	t.Errorf("doesn't allow requests to resume after reset. had to wait %d milliseconds", wait.Milliseconds())
	t.Errorf("doesn't allow requests to resume after reset. had to wait %s", wait)

-		url, _ := url.Parse(endpoint)
+		url, err := url.Parse(endpoint)
+		if err != nil {
+			t.Fatalf("failed to parse endpoint: %v", err)
+		}

.../input/entityanalytics/provider/okta: Rate limiting fix, improvements #41977

.../input/entityanalytics/provider/okta: Rate limiting fix, improvements #41977

Conversation

chrisberkhout commented Dec 10, 2024 • edited Loading

Proposed commit message

Checklist

Related issues

elasticmachine commented Dec 10, 2024

mergify bot commented Dec 10, 2024

mergify bot commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisberkhout commented Dec 11, 2024

mergify bot commented Dec 11, 2024

efd6 left a comment

Choose a reason for hiding this comment

chrisberkhout commented Dec 10, 2024 •

edited

Loading